feat: add workspace tabs — nested tab layer within each workspace#2234
Open
oobagi wants to merge 3 commits intomanaflow-ai:mainfrom
Open
feat: add workspace tabs — nested tab layer within each workspace#2234oobagi wants to merge 3 commits intomanaflow-ai:mainfrom
oobagi wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
Adds a tab layer between workspaces (sidebar) and split panes, allowing each workspace to have multiple independent layouts. For example, one tab can hold a fullscreen browser while another has a 6-pane terminal grid. Architecture: - New WorkspaceTab class extracts BonsplitController + panel state from Workspace - Workspace delegates panel operations through computed pass-throughs - BonsplitViews rendered in a ZStack (opacity toggle) to avoid AppKit portal ghosting - BonsplitDelegate callbacks resolve owning tab via controller identity (===) - panelsPublisher uses workspace-level activePanels mirror to stay connected across tab switches - All ~80 force-unwraps replaced with activeWorkspaceTab (descriptive fatalError) Features: - Tab bar auto-hides with 1 tab, appears with 2+ - Keyboard shortcuts: ⌥⌘] (next), ⌥⌘[ (prev), ⌥⌘T (new), ⌥⌘R (rename), ⌥⌘W (close) - Command palette integration (5 commands, conditional visibility) - Rename uses existing command palette rename flow - Session persistence with backward-compatible optional fields - Tab bar tooltips show customizable shortcut hints - No layout shift on close button hover (opacity toggle) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@oobagi is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds multi-tab workspace support: new WorkspaceTab model and tab bar views, refactors Workspace/TabManager for per-tab state and session persistence, wires workspace-tab keyboard shortcuts and command‑palette rename flow, and switches panel observation to panelsPublisher. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentView
participant TabManager
participant Workspace
participant WorkspaceTab
User->>ContentView: Click "New Tab" / press shortcut
ContentView->>TabManager: createWorkspaceTabInSelectedWorkspace()
TabManager->>Workspace: createWorkspaceTab()
Workspace->>WorkspaceTab: init(bonsplitController)
Workspace->>Workspace: append workspaceTabs, syncActivePanels()
Workspace-->>ContentView: UI updates (workspace.panelsPublisher)
User->>ContentView: Select tab
ContentView->>Workspace: selectWorkspaceTab(at:)
Workspace->>Workspace: set selectedWorkspaceTabIndex, syncActivePanels()
sequenceDiagram
participant User
participant AppDelegate
participant NotificationCenter
participant ContentView
participant Workspace
participant WorkspaceTab
User->>AppDelegate: Press "rename workspace tab" shortcut
AppDelegate->>NotificationCenter: post commandPaletteRenameWorkspaceTabRequested
NotificationCenter->>ContentView: notification received
ContentView->>ContentView: openCommandPaletteRenameWorkspaceTabInput()
ContentView->>Workspace: beginRenameWorkspaceTabFlow()
Workspace->>WorkspaceTab: set customTitle on confirm
WorkspaceTab-->>ContentView: published changes -> UI updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds workspace tabs — a nested tab layer sitting between the workspace sidebar and the split-pane layer — so each workspace can host multiple independent layouts. The implementation is large (~1400 lines net) and well-executed: state is cleanly extracted into a new Confidence Score: 4/5Safe to merge; no data-loss or correctness bugs found. The three flagged items are all cosmetic/UX-level P2s. Core correctness is solid: BonsplitDelegate ownership, publisher forwarding, session persistence, and close-confirmation async safety are all handled correctly. The ignoresSafeArea placement in minimal mode is the one concern that could produce a visible layout regression (tab bar appearing under the titlebar), but only when 2+ workspace tabs are open in minimal mode — a relatively narrow scenario. The other two comments are quality-of-life improvements rather than blockers. Prior concerns from the thread (UUID sort, ?? UUID() allocation) were addressed in this version. Score of 4 reflects one targeted fix recommended before shipping. Sources/WorkspaceContentView.swift (ignoresSafeArea scope in minimal mode), Sources/WorkspaceTabBarView.swift (tab bar overflow) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Workspace["Workspace (@MainActor, ObservableObject)"]
WT["workspaceTabs: [WorkspaceTab]"]
IDX["selectedWorkspaceTabIndex: Int @Published"]
AP["activePanels: [UUID:Panel] @Published"]
SYNC["syncActivePanels()"]
PT["Computed pass-throughs\nbonsplitController, panels,\npanelDirectories, etc."]
end
subgraph WorkspaceTab1["WorkspaceTab (selected)"]
BC1["BonsplitController"]
P1["panels: [UUID:Panel] @Published"]
end
subgraph WorkspaceTab2["WorkspaceTab (background)"]
BC2["BonsplitController (isInteractive=false)"]
P2["panels: [UUID:Panel] @Published"]
end
IDX -->|"resolves via activeWorkspaceTab"| PT
PT -->|"reads/writes"| WorkspaceTab1
SYNC -->|"subscribes to tab.$panels"| P1
P1 -->|"mirrors into"| AP
AP -->|"panelsPublisher = $activePanels"| Outside["TabManager / AppDelegate\nContentView subscribers"]
IDX -->|"selectWorkspaceTab(at:)\ncalls syncActivePanels()"| SYNC
subgraph UI["WorkspaceContentView"]
ZS["ZStack\nForEach workspaceTabs"]
BSV1["BonsplitView(controller: tab1.bc)\nopacity=1, interactive=true"]
BSV2["BonsplitView(controller: tab2.bc)\nopacity=0, interactive=false"]
end
BC1 --> BSV1
BC2 --> BSV2
BSV1 & BSV2 --> ZS
subgraph TabBar["WorkspaceTabBarView"]
TB["HStack of WorkspaceTabItemView\nauto-hides if count <= 1"]
end
Workspace -->|"@ObservedObject"| TabBar
Workspace -->|"isWorkspaceInputActive\nselectedWorkspaceTabIndex"| UI
Reviews (2): Last reviewed commit: "fix: address code review bot feedback on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+72
to
+83
| if !basename.isEmpty { | ||
| return basename | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Try panel titles | ||
| for panelId in tab.panels.keys.sorted(by: { $0.uuidString < $1.uuidString }) { | ||
| if let title = tab.panelTitles[panelId]?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !title.isEmpty { | ||
| return title | ||
| } |
Contributor
There was a problem hiding this comment.
tab.panels.keys.sorted(by: { $0.uuidString < $1.uuidString }) sorts by UUID string, which is effectively random from a user-visible perspective. This means the tab title (directory basename or panel title) is derived from whichever panel happens to sort first alphabetically — not the focused panel, not the first created panel, just a random one. In practice this can look like the tab title flickers to a different panel's name whenever panels are added or removed.
A more intuitive approach would be to prefer the focused panel's directory/title, falling back to the first-opened panel's. WorkspaceTab already tracks focusedPanelId for exactly this purpose:
// Prefer the focused panel's directory/title
if let focusedId = tab.focusedPanelId {
if let dir = tab.panelDirectories[focusedId], !dir.isEmpty {
let base = (dir as NSString).lastPathComponent
if !base.isEmpty { return base }
}
if let title = tab.panelTitles[focusedId]?.trimmingCharacters(in: .whitespacesAndNewlines),
!title.isEmpty {
return title
}
}
// Fall back to any panel (stable arbitrary order is fine as a last resort)
for panelId in tab.panels.keys.sorted(by: { $0.uuidString < $1.uuidString }) { ... }| BonsplitView(controller: wsTab.bonsplitController) { tab, paneId in | ||
| let _ = Self.debugPanelLookup(tab: tab, workspace: workspace) | ||
| if let panel = wsTab.panels[wsTab.surfaceIdToPanelId[tab.id] ?? UUID()] { | ||
| let isFocused = isWorkspaceInputActive && isSelectedTab && workspace.focusedPanelId == panel.id |
Contributor
There was a problem hiding this comment.
?? UUID() allocates a throwaway UUID on every render
wsTab.panels[wsTab.surfaceIdToPanelId[tab.id] ?? UUID()] creates a fresh UUID() object on every call — once per panel per render frame — just so the outer if let can evaluate to nil. The allocation is harmless but it's an unusual pattern that can confuse readers into thinking the UUID() is significant. Use two-level optional chaining instead:
Suggested change
| let isFocused = isWorkspaceInputActive && isSelectedTab && workspace.focusedPanelId == panel.id | |
| if let surfacePanelId = wsTab.surfaceIdToPanelId[tab.id], | |
| let panel = wsTab.panels[surfacePanelId] { |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:459">
P2: createBareWorkspaceTab doesn't close Bonsplit's default "Welcome" tab, so restored tabs can retain an extra placeholder tab that restoreWorkspaceTabFromSnapshot never removes.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/WorkspaceContentView.swift (1)
386-412:⚠️ Potential issue | 🟠 MajorSync notification badges for all workspace tabs, not just the selected one.
syncBonsplitNotificationBadges()currently updates badges only forworkspace.bonsplitController(the selected tab). Since all tabs remain alive in the ZStack, non-selected tabs' badge states become stale when notifications change or manual unread status updates.Iterate
workspace.workspaceTabsto sync badges across all tab controllers, similar to the pattern already used for settingisInteractiveandonFileDropper tab.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceContentView.swift` around lines 386 - 412, syncBonsplitNotificationBadges only updates tabs from workspace.bonsplitController (the selected controller), leaving other controllers' tabs stale; change the function to iterate all controllers via workspace.workspaceTabs and run the same badge/pin/kind logic for each tab there. Locate syncBonsplitNotificationBadges and replace the outer loop over workspace.bonsplitController.allPaneIds with iteration over workspace.workspaceTabs (or the collection used elsewhere for isInteractive/onFileDrop), compute panelId using workspace.panelIdFromSurfaceId(tab.id), expectedKind via workspace.panelKind(panelId:), expectedPinned via workspace.isPanelPinned(_), and shouldShow using notificationStore.hasVisibleNotificationIndicator(forTabId:workspace.id, surfaceId:) || workspace.manualUnreadPanelIds; call bonsplitController.updateTab (or the specific controller's updateTab) for each tab to apply kind, showsNotificationBadge and isPinned updates.Sources/Workspace.swift (1)
10372-10403:⚠️ Potential issue | 🟠 MajorThis confirmation path still consults the selected tab, not
owningTab.
panelIdcomes fromowningTab, butterminalPanel(for:)andpanelNeedsConfirmClose(...)both read throughactiveWorkspaceTab. If a non-selected controller fires this callback, the guard falls through and a running terminal can close without confirmation. Use tab-scoped state here instead of the active-tab helpers.Suggested direction
- guard let panelId = owningTab.surfaceIdToPanelId[tab.id], - let terminalPanel = terminalPanel(for: panelId) else { + guard let panelId = owningTab.surfaceIdToPanelId[tab.id], + let terminalPanel = owningTab.panels[panelId] as? TerminalPanel else { stageClosedBrowserRestoreSnapshotIfNeeded(for: tab, inPane: pane) recordPostCloseSelection() return true } - if panelNeedsConfirmClose(panelId: panelId, fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose()) { + if Self.resolveCloseConfirmation( + shellActivityState: owningTab.panelShellActivityStates[panelId], + fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose() + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10372 - 10403, The confirmation branch incorrectly uses helpers that consult the selected/active workspace tab (e.g., terminalPanel(for:) and panelNeedsConfirmClose(...)) instead of the caller's tab-scoped state, so a non-selected controller can bypass confirmation; update the code to resolve the terminal panel and confirmation check from owningTab's scope: use the panelId you already pulled from owningTab.surfaceIdToPanelId[tab.id] to look up the terminal panel via an owningTab-scoped lookup (rather than terminalPanel(for:)) and call a confirmation check that uses owningTab state (or call terminalPanel.needsConfirmClose() on the panel instance obtained from owningTab) so panelNeedsConfirmClose(...) does not consult activeWorkspaceTab; keep the existing flow of inserting into owningTab.pendingCloseConfirmTabIds, dispatching the async Task, and using owningTab.forceCloseTabIds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 9415-9418: The newWorkspaceTab shortcut is shadowing the legacy
⌥⌘T handler; update either KeyboardShortcutSettings.shortcut(for:
.newWorkspaceTab) to a different default or remove/remap the new handler so
AppDelegate.handleCustomShortcut(event:) can still route ⌥⌘T to
closeOtherTabsInFocusedPaneWithConfirmation(); locate the matchShortcut(...)
check around matchShortcut(event: event, shortcut:
KeyboardShortcutSettings.shortcut(for: .newWorkspaceTab)) and the
tabManager?.createWorkspaceTabInSelectedWorkspace() call and change the default
binding or delete/adjust that branch so the legacy
closeOtherTabsInFocusedPaneWithConfirmation() path remains reachable.
In `@Sources/cmuxApp.swift`:
- Around line 790-800: The default shortcut for
KeyboardShortcutSettings.Action.newWorkspaceTab (currently ⌥⌘T) conflicts with
other bindings; change its defaultShortcut to a non-conflicting combo (e.g.
Option-Command-N) by updating
KeyboardShortcutSettings.Action.newWorkspaceTab.defaultShortcut, and ensure the
splitCommandButton usage in cmuxApp.swift (the "New Workspace Tab" call) and any
hard-coded handling in AppDelegate.handleCustomShortcut are consistent with the
new default so the ambiguous ⌥⌘T routing no longer masks this action.
In `@Sources/ContentView.swift`:
- Around line 5307-5310: workspaceTabSubtitle is using
CommandPaletteContextKeys.workspaceName (the parent workspace) instead of the
active tab's title, so workspace-targeting commands become ambiguous; change
workspaceTabSubtitle to read the active workspace-tab name from the command
palette context (e.g., a key like CommandPaletteContextKeys.workspaceTabName or
selectedWorkspaceTabTitle) and use that as the subtitle (falling back to the
workspace name or your existing "Workspace" fallback if the tab name is
missing), and apply the same replacement for the other relevant helpers in the
6004-6053 region so Close/Next/Previous/Rename Workspace Tab display the active
tab name rather than the workspace name.
- Around line 7362-7374: The rename flow for workspace tabs currently seeds the
input with wsTab.customTitle ?? "" which leaves untitled tabs empty; change
beginRenameWorkspaceTabFlow to seed using the tab's displayed title (use the
same property other flows use, e.g. wsTab.title or a displayTitle getter)
instead of wsTab.customTitle so the field pre-fills the current visible name;
update the CommandPaletteRenameTarget initialization to pass that displayed
title as currentName and leave the rest (workspace.selectedWorkspaceTab,
startRenameFlow) unchanged.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 205-206: The enum case newWorkspaceTab currently returns a
hard-coded StoredShortcut (key: "t", command: true, option: true) which
conflicts with the existing hard-coded ⌥⌘T handler for
closeOtherTabsInFocusedPaneWithConfirmation; update the shortcut wiring so the
handlers don't shadow each other by either assigning newWorkspaceTab a different
default StoredShortcut (change the StoredShortcut returned in newWorkspaceTab)
or remove the hard-coded ⌥⌘T handling and instead route
closeOtherTabsInFocusedPaneWithConfirmation through the settings system; locate
the StoredShortcut return in the newWorkspaceTab case and the
closeOtherTabsInFocusedPaneWithConfirmation handler and make the change
consistently so only one handler matches ⌥⌘T at runtime.
In `@Sources/TabManager.swift`:
- Around line 3255-3275: The sessionAutosaveFingerprint() currently only
includes active workspace pass-throughs so mutations to non-selected workspace
tabs (created/closed via methods like createWorkspaceTabInSelectedWorkspace,
closeSelectedWorkspaceTab, selectNextWorkspaceTab, selectPreviousWorkspaceTab)
can change the persisted session without updating the fingerprint; update
sessionAutosaveFingerprint() to incorporate the nested workspace-tab state (each
Workspace's tabs collection and the selected tab index) or simply derive the
fingerprint from the full session snapshot used for persistence (e.g. compute
the hash over session snapshot data rather than only active pass-throughs) so
tab-bar changes in any workspace will flip the dirty check consistently.
In `@Sources/Workspace.swift`:
- Around line 302-306: The snapshot created in make/save path omits tab title
data so renamed workspace tabs don't persist; update SessionWorkspaceTabSnapshot
to include title and customTitle (or similar properties), then update the code
that constructs the snapshot (the return of SessionWorkspaceTabSnapshot(...) in
Workspace.swift) to pass the current tab's title/custom title values, and ensure
corresponding decoding/restore logic uses these new fields when restoring tabs
so names round-trip correctly.
- Around line 5956-6004: closeWorkspaceTab currently force-closes panels by
calling panel.close() and manually shredding tab dictionaries, bypassing the
centralized close/confirmation pipeline and Bonsplit callbacks (e.g.,
untrackRemoteTerminalSurface, notification clearing, closed-browser staging).
Instead, iterate the tab's panel IDs and invoke the workspace/registry-level
panel close function that runs the full close pipeline (the same code path used
elsewhere — e.g., the Bonsplit/Workspace method that handles surface/panel
closing and confirmations), so per-surface side-effects and confirmations run;
remove the ad-hoc dictionary removals tied to panel lifecycle (leaving only
tab-level cleanup after the pipeline completes); ensure
PortScanner.shared.unregisterPanel and workspaceTabObjectWillChangeCancellables
are updated after the centralized close completes, then call syncActivePanels()
and reassertWorkspaceTabFocus() as currently done.
- Around line 397-405: After setting selectedWorkspaceTabIndex you must reapply
the focus-derived workspace mirrors (currentDirectory, gitBranch, pullRequest,
focus) for the newly selected tab so they reflect the actual selected tab rather
than the last-restored tab; update the block that currently only calls
syncActivePanels() to also invoke the same mirror-refresh logic used in
restoreWorkspaceTabFromSnapshot(...) (or extract that mirror-update code into a
helper and call it here) so that selectedWorkspaceTabIndex,
restoreWorkspaceTabFromSnapshot, syncActivePanels, and the workspace mirror
properties are all kept in sync.
In `@Sources/WorkspaceContentView.swift`:
- Around line 259-265: The closure assigned to bonsplitController.onFileDrop
captures wsTab strongly causing a retain cycle; change the capture list to
capture wsTab weakly (e.g. [weak wsTab, weak workspace]) and update the guard
statements to unwrap the weak wsTab before accessing surfaceIdToPanelId and
panels, then call panel.hostedView.handleDroppedURLs(urls) on the unwrapped
wsTab's panel; ensure TerminalPanel casting and selectedTab lookup remain the
same but operate on the unwrapped weak wsTab to break the cycle.
In `@Sources/WorkspaceTabBarView.swift`:
- Line 86: Replace the hard-coded fallback title "New Tab" in
WorkspaceTabBarView with a localized string using the required API; locate the
computed property or method that currently returns "New Tab" and change it to
something like String(localized: "workspaceTabBar.newTab", defaultValue: "New
Tab") so the UI text is localized (use the key naming scheme your project uses).
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 10372-10403: The confirmation branch incorrectly uses helpers that
consult the selected/active workspace tab (e.g., terminalPanel(for:) and
panelNeedsConfirmClose(...)) instead of the caller's tab-scoped state, so a
non-selected controller can bypass confirmation; update the code to resolve the
terminal panel and confirmation check from owningTab's scope: use the panelId
you already pulled from owningTab.surfaceIdToPanelId[tab.id] to look up the
terminal panel via an owningTab-scoped lookup (rather than terminalPanel(for:))
and call a confirmation check that uses owningTab state (or call
terminalPanel.needsConfirmClose() on the panel instance obtained from owningTab)
so panelNeedsConfirmClose(...) does not consult activeWorkspaceTab; keep the
existing flow of inserting into owningTab.pendingCloseConfirmTabIds, dispatching
the async Task, and using owningTab.forceCloseTabIds.
In `@Sources/WorkspaceContentView.swift`:
- Around line 386-412: syncBonsplitNotificationBadges only updates tabs from
workspace.bonsplitController (the selected controller), leaving other
controllers' tabs stale; change the function to iterate all controllers via
workspace.workspaceTabs and run the same badge/pin/kind logic for each tab
there. Locate syncBonsplitNotificationBadges and replace the outer loop over
workspace.bonsplitController.allPaneIds with iteration over
workspace.workspaceTabs (or the collection used elsewhere for
isInteractive/onFileDrop), compute panelId using
workspace.panelIdFromSurfaceId(tab.id), expectedKind via
workspace.panelKind(panelId:), expectedPinned via workspace.isPanelPinned(_),
and shouldShow using
notificationStore.hasVisibleNotificationIndicator(forTabId:workspace.id,
surfaceId:) || workspace.manualUnreadPanelIds; call bonsplitController.updateTab
(or the specific controller's updateTab) for each tab to apply kind,
showsNotificationBadge and isPinned updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d266356-e3eb-4e67-b6c0-dc773d0214ad
📒 Files selected for processing (12)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftSources/WorkspaceTab.swiftSources/WorkspaceTabBarView.swiftSources/cmuxApp.swiftcmuxTests/SessionPersistenceTests.swift
Comment on lines
+790
to
+800
| splitCommandButton(title: String(localized: "menu.view.nextWorkspaceTab", defaultValue: "Next Workspace Tab"), shortcut: nextWorkspaceTabMenuShortcut) { | ||
| activeTabManager.selectNextWorkspaceTab() | ||
| } | ||
|
|
||
| splitCommandButton(title: String(localized: "menu.view.previousWorkspaceTab", defaultValue: "Previous Workspace Tab"), shortcut: prevWorkspaceTabMenuShortcut) { | ||
| activeTabManager.selectPreviousWorkspaceTab() | ||
| } | ||
|
|
||
| splitCommandButton(title: String(localized: "menu.view.newWorkspaceTab", defaultValue: "New Workspace Tab"), shortcut: newWorkspaceTabMenuShortcut) { | ||
| activeTabManager.createWorkspaceTabInSelectedWorkspace() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check new workspace-tab shortcut default:"
rg -n --type=swift -C3 'case\s+newWorkspaceTab|newWorkspaceTab.*defaultShortcut|StoredShortcut\(.*"t".*\.option' Sources/KeyboardShortcutSettings.swift
echo
echo "2) Check existing Cmd+Option+T bindings in app/menu routing:"
rg -n --type=swift -C3 'keyboardShortcut\("t", modifiers:\s*\[\.command,\s*\.option\]\)|closeOtherTabsInFocusedPaneWithConfirmation|handleCustomShortcut' Sources/cmuxApp.swift Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 6864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for newWorkspaceTab defaultShortcut definition ==="
rg -n --type=swift -A10 'case newWorkspaceTab' Sources/KeyboardShortcutSettings.swift
echo
echo "=== Check around lines 790-800 in cmuxApp.swift for shortcut binding ==="
sed -n '780,810p' Sources/cmuxApp.swift | cat -n
echo
echo "=== Search for newWorkspaceTab in handleCustomShortcut ==="
rg -n --type=swift -B2 -A5 'newWorkspaceTab|createWorkspaceTabInSelectedWorkspace' Sources/AppDelegate.swift | head -50Repository: manaflow-ai/cmux
Length of output: 3065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find newWorkspaceTabMenuShortcut variable definition ==="
rg -n --type=swift 'let.*newWorkspaceTabMenuShortcut|var.*newWorkspaceTabMenuShortcut' Sources/
echo
echo "=== Find default shortcut mappings in KeyboardShortcutSettings ==="
rg -n --type=swift 'defaultShortcut|case newWorkspaceTab' Sources/KeyboardShortcutSettings.swift | head -30
echo
echo "=== Search for StoredShortcut or defaultShortcut patterns ==="
rg -n --type=swift 'StoredShortcut.*\.option.*\.t|\"t\".*option|newWorkspaceTab.*default' Sources/KeyboardShortcutSettings.swiftRepository: manaflow-ai/cmux
Length of output: 814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show newWorkspaceTabMenuShortcut definition in cmuxApp.swift ==="
sed -n '1008,1015p' Sources/cmuxApp.swift | cat -n
echo
echo "=== Show defaultShortcut property and newWorkspaceTab case in KeyboardShortcutSettings ==="
sed -n '141,200p' Sources/KeyboardShortcutSettings.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 5129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Continue reading defaultShortcut cases to find newWorkspaceTab ==="
sed -n '141,250p' Sources/KeyboardShortcutSettings.swift | cat -n
echo
echo "=== Or search specifically for newWorkspaceTab case ==="
rg -n --type=swift -A1 'case newWorkspaceTab' Sources/KeyboardShortcutSettings.swiftRepository: manaflow-ai/cmux
Length of output: 7537
Resolve default shortcut collision for "New Workspace Tab".
KeyboardShortcutSettings.Action.newWorkspaceTab.defaultShortcut is ⌥⌘T, which conflicts with the "Close Other Tabs in Pane" shortcut at Sources/cmuxApp.swift:645 and the hard-coded ⌥⌘T routing in AppDelegate.handleCustomShortcut() (line 9448). This creates ambiguous behavior where one action masks the other.
Assign a non-conflicting default to newWorkspaceTab (similar to how toggleTextBoxInput uses ⌥⌘B to avoid this same collision).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 790 - 800, The default shortcut for
KeyboardShortcutSettings.Action.newWorkspaceTab (currently ⌥⌘T) conflicts with
other bindings; change its defaultShortcut to a non-conflicting combo (e.g.
Option-Command-N) by updating
KeyboardShortcutSettings.Action.newWorkspaceTab.defaultShortcut, and ensure the
splitCommandButton usage in cmuxApp.swift (the "New Workspace Tab" call) and any
hard-coded handling in AppDelegate.handleCustomShortcut are consistent with the
new default so the ambiguous ⌥⌘T routing no longer masks this action.
Comment on lines
+5307
to
+5310
| func workspaceTabSubtitle(_ context: CommandPaletteContextSnapshot) -> String { | ||
| let name = context.string(CommandPaletteContextKeys.workspaceName) ?? String(localized: "commandPalette.subtitle.workspaceFallback", defaultValue: "Workspace") | ||
| return String(localized: "commandPalette.subtitle.workspaceTabWithName", defaultValue: "Workspace Tab • \(name)") | ||
| } |
There was a problem hiding this comment.
Make workspace-tab commands identify the active tab.
workspaceTabSubtitle currently renders the parent workspace name, so Close/Next/Previous/Rename Workspace Tab all describe the workspace instead of the selected workspace tab. In a multi-tab workspace that makes the command target ambiguous.
Also applies to: 6004-6053
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 5307 - 5310, workspaceTabSubtitle is
using CommandPaletteContextKeys.workspaceName (the parent workspace) instead of
the active tab's title, so workspace-targeting commands become ambiguous; change
workspaceTabSubtitle to read the active workspace-tab name from the command
palette context (e.g., a key like CommandPaletteContextKeys.workspaceTabName or
selectedWorkspaceTabTitle) and use that as the subtitle (falling back to the
workspace name or your existing "Workspace" fallback if the tab name is
missing), and apply the same replacement for the other relevant helpers in the
6004-6053 region so Close/Next/Previous/Rename Workspace Tab display the active
tab name rather than the workspace name.
Comment on lines
+5956
to
+6004
| func closeWorkspaceTab(at index: Int) { | ||
| guard workspaceTabs.count > 1, workspaceTabs.indices.contains(index) else { return } | ||
|
|
||
| let tab = workspaceTabs[index] | ||
|
|
||
| // Tear down all panels in the tab being closed. | ||
| let panelEntries = Array(tab.panels) | ||
| for (panelId, panel) in panelEntries { | ||
| tab.panelSubscriptions.removeValue(forKey: panelId) | ||
| PortScanner.shared.unregisterPanel(workspaceId: id, panelId: panelId) | ||
| panel.close() | ||
| } | ||
| tab.panels.removeAll(keepingCapacity: false) | ||
| tab.surfaceIdToPanelId.removeAll(keepingCapacity: false) | ||
| tab.panelSubscriptions.removeAll(keepingCapacity: false) | ||
| tab.panelDirectories.removeAll(keepingCapacity: false) | ||
| tab.panelTitles.removeAll(keepingCapacity: false) | ||
| tab.panelCustomTitles.removeAll(keepingCapacity: false) | ||
| tab.panelGitBranches.removeAll(keepingCapacity: false) | ||
| tab.panelPullRequests.removeAll(keepingCapacity: false) | ||
| tab.pinnedPanelIds.removeAll() | ||
| tab.manualUnreadPanelIds.removeAll() | ||
| tab.manualUnreadMarkedAt.removeAll(keepingCapacity: false) | ||
| tab.surfaceListeningPorts.removeAll(keepingCapacity: false) | ||
| tab.surfaceTTYNames.removeAll(keepingCapacity: false) | ||
| tab.panelShellActivityStates.removeAll(keepingCapacity: false) | ||
| tab.restoredTerminalScrollbackByPanelId.removeAll(keepingCapacity: false) | ||
| tab.terminalInheritanceFontPointsByPanelId.removeAll(keepingCapacity: false) | ||
| tab.lastTerminalConfigInheritancePanelId = nil | ||
| tab.lastTerminalConfigInheritanceFontPoints = nil | ||
|
|
||
| // Remove the objectWillChange forwarding subscription. | ||
| workspaceTabObjectWillChangeCancellables.removeValue(forKey: tab.id) | ||
|
|
||
| // Remove from the array. | ||
| workspaceTabs.remove(at: index) | ||
|
|
||
| // Adjust selected index: if we closed the selected tab or one before it, | ||
| // keep the selection valid. | ||
| if selectedWorkspaceTabIndex >= workspaceTabs.count { | ||
| selectedWorkspaceTabIndex = workspaceTabs.count - 1 | ||
| } else if index < selectedWorkspaceTabIndex { | ||
| selectedWorkspaceTabIndex -= 1 | ||
| } | ||
| // If we closed the selected tab itself, the index now points to the | ||
| // tab that slid into its position (or the one before it if it was last). | ||
| // Either way, sync the panels publisher and reassert focus. | ||
| syncActivePanels() | ||
| reassertWorkspaceTabFocus() |
There was a problem hiding this comment.
closeWorkspaceTab is bypassing the normal close pipeline.
This bulk teardown hard-closes every panel with panel.close() and manual dictionary cleanup, so terminals in that workspace tab never go through the existing confirmation path. It also skips the per-surface side effects already centralized in the Bonsplit callbacks (untrackRemoteTerminalSurface, notification clearing, closed-browser staging, etc.), which leaves workspace-level state stale after ⌥⌘W.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 5956 - 6004, closeWorkspaceTab
currently force-closes panels by calling panel.close() and manually shredding
tab dictionaries, bypassing the centralized close/confirmation pipeline and
Bonsplit callbacks (e.g., untrackRemoteTerminalSurface, notification clearing,
closed-browser staging). Instead, iterate the tab's panel IDs and invoke the
workspace/registry-level panel close function that runs the full close pipeline
(the same code path used elsewhere — e.g., the Bonsplit/Workspace method that
handles surface/panel closing and confirmations), so per-surface side-effects
and confirmations run; remove the ad-hoc dictionary removals tied to panel
lifecycle (leaving only tab-level cleanup after the pipeline completes); ensure
PortScanner.shared.unregisterPanel and workspaceTabObjectWillChangeCancellables
are updated after the centralized close completes, then call syncActivePanels()
and reassertWorkspaceTabFocus() as currently done.
Fixes all critical, major, and minor issues from CodeRabbit, Greptile, and Cubic reviews: Critical: - panels pass-through now iterates all workspace tabs for workspace-wide operations (needsConfirmClose, teardownAllPanels, hidePortalViews, shouldCloseWorkspaceOnLastSurface) - closeWorkspaceTab now checks for running processes and shows confirmation dialog before closing, with full cleanup side effects (untrackRemoteTerminalSurface, clearNotifications) Major: - Resolved ⌥⌘T shortcut collision: "Close Other Tabs in Pane" moved to ⌥⌘X and is now user-configurable via KeyboardShortcutSettings - Fixed retain cycle in onFileDrop closure (weak wsTab capture) - Session autosave fingerprint now includes workspace tab count and selected index - Custom tab titles now persist across session save/restore - Bare-tab restore cleans up Bonsplit Welcome tabs Minor: - Tab title prefers focused panel's directory over UUID-sort - Fallback "New Tab" string localized - Rename flow prefills with derived title instead of empty string Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:5999">
P2: Workspace-tab command entries hardcode shortcut hints, but commandPaletteShortcutAction doesn’t map the new workspace-tab command IDs to KeyboardShortcutSettings.Action, so the palette won’t reflect user-customized shortcuts and will keep showing these defaults.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:5371">
P2: Workspace-level Bonsplit appearance/tooltip updates now affect only the active workspace tab controller, causing config drift across workspace tabs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| commandId: "palette.newWorkspaceTab", | ||
| title: constant(String(localized: "command.newWorkspaceTab.title", defaultValue: "New Workspace Tab")), | ||
| subtitle: workspaceTabSubtitle, | ||
| shortcutHint: "⌥⌘T", |
There was a problem hiding this comment.
P2: Workspace-tab command entries hardcode shortcut hints, but commandPaletteShortcutAction doesn’t map the new workspace-tab command IDs to KeyboardShortcutSettings.Action, so the palette won’t reflect user-customized shortcuts and will keep showing these defaults.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 5999:
<comment>Workspace-tab command entries hardcode shortcut hints, but commandPaletteShortcutAction doesn’t map the new workspace-tab command IDs to KeyboardShortcutSettings.Action, so the palette won’t reflect user-customized shortcuts and will keep showing these defaults.</comment>
<file context>
@@ -5960,6 +5989,69 @@ struct ContentView: View {
+ commandId: "palette.newWorkspaceTab",
+ title: constant(String(localized: "command.newWorkspaceTab.title", defaultValue: "New Workspace Tab")),
+ subtitle: workspaceTabSubtitle,
+ shortcutHint: "⌥⌘T",
+ keywords: ["new", "workspace", "tab", "create"],
+ when: { $0.bool(CommandPaletteContextKeys.hasWorkspace) }
</file context>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
Sources/ContentView.swift (1)
5241-5244:⚠️ Potential issue | 🟡 MinorCapture the active workspace-tab name in the palette context.
Lines 5241-5244 only record the tab count, and Line 5308 still reads
workspaceName. That leaves the new Close/Next/Previous/Rename Workspace Tab entries labeled with the parent workspace instead of the selected workspace tab, which is ambiguous in multi-tab workspaces.Suggested fix
static let workspaceHasMultipleTabs = "workspace.hasMultipleTabs" + static let workspaceSelectedTabName = "workspace.selectedTabName" @@ snapshot.setBool( CommandPaletteContextKeys.workspaceHasMultipleTabs, workspace.workspaceTabs.count > 1 ) + snapshot.setString( + CommandPaletteContextKeys.workspaceSelectedTabName, + workspace.selectedWorkspaceTab.map { + ($0.customTitle ?? $0.derivedTitle) + .trimmingCharacters(in: .whitespacesAndNewlines) + } + ) @@ func workspaceTabSubtitle(_ context: CommandPaletteContextSnapshot) -> String { - let name = context.string(CommandPaletteContextKeys.workspaceName) ?? String(localized: "commandPalette.subtitle.workspaceFallback", defaultValue: "Workspace") + let name = context.string(CommandPaletteContextKeys.workspaceSelectedTabName) + ?? context.string(CommandPaletteContextKeys.workspaceName) + ?? String(localized: "commandPalette.subtitle.workspaceFallback", defaultValue: "Workspace") return String(localized: "commandPalette.subtitle.workspaceTabWithName", defaultValue: "Workspace Tab • \(name)") }Also applies to: 5307-5310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 5241 - 5244, The palette context currently only records tab count via snapshot.setBool(CommandPaletteContextKeys.workspaceHasMultipleTabs, workspace.workspaceTabs.count > 1) so commands still display the parent workspaceName instead of the selected tab; update the context to also capture the active workspace-tab name (e.g., set a new or existing key like CommandPaletteContextKeys.workspaceTabName or similar) with the selected tab's name (derived from workspace.workspaceTabs[workspace.activeTabIndex] or the workspace.activeTab property) and ensure the same change is applied where workspaceName is read (around the code that handles the Close/Next/Previous/Rename Workspace Tab entries at the 5307-5310 area) so those commands show the selected tab name instead of the parent workspace.Sources/Workspace.swift (3)
5376-5389:⚠️ Potential issue | 🔴 CriticalSelected-tab pass-through state is still leaking into non-selected contexts.
After
panelsand the panel metadata stores became pass-throughs toactiveWorkspaceTab, several callers below still use them from workspace-wide or controller-scoped paths. In this file that means background tabs can miss remote proxy/status propagation, and delegate flows that resolveowningTabcan still bounce back through selected-tab helpers liketerminalPanel(for:),browserPanel(for:), orresolvedPanelTitle(...), reintroducing cross-tab corruption.Also applies to: 5463-5563
6087-6102:⚠️ Potential issue | 🟠 MajorWorkspace-tab activation never refreshes focus-derived mirrors.
syncActivePanels()only swaps the publisher andreassertWorkspaceTabFocus()only nudges responder state. After tab selection, tab-close, and restore reuse this pair,currentDirectory,gitBranch, andpullRequestcan stay pointed at the previously active workspace tab until some later Bonsplit callback happens.💡 Reuse the existing tab-selection path here
private func reassertWorkspaceTabFocus() { guard let tab = selectedWorkspaceTab else { return } - // Find the focused panel in the selected tab. guard let focusedPaneId = tab.bonsplitController.focusedPaneId, - let selectedBonsplitTab = tab.bonsplitController.selectedTab(inPane: focusedPaneId), - let panelId = tab.surfaceIdToPanelId[selectedBonsplitTab.id], - let panel = tab.panels[panelId] else { return } - - panel.focus() - - // For terminal panels, explicitly move AppKit first responder. - if let terminalPanel = panel as? TerminalPanel { - terminalPanel.hostedView.ensureFocus(for: id, surfaceId: panelId) - } + let selectedBonsplitTab = tab.bonsplitController.selectedTab(inPane: focusedPaneId) else { return } + + applyTabSelection(tabId: selectedBonsplitTab.id, inPane: focusedPaneId) }Also applies to: 6104-6121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 6087 - 6102, syncActivePanels() replaces the activePanels publisher but doesn't refresh focus-derived mirrors (currentDirectory, gitBranch, pullRequest); update it to re-use the existing tab-selection path by calling the same focus-refresh routine used on tab selection (e.g. invoke reassertWorkspaceTabFocus() or the shared tab-selection handler) immediately after swapping tab and setting activePanels so responder-derived mirrors are recomputed; ensure the same change is applied in the sibling block noted (the duplicate at 6104-6121) so both paths keep mirror state in sync when tabs are switched/closed/restored.
240-243:⚠️ Potential issue | 🟠 MajorPersist renamed single workspace tabs too.
workspaceTabsis only emitted whenworkspaceTabs.count > 1, so a lone workspace tab'scustomTitlenever reachesSessionWorkspaceTabSnapshot. The top-levelcustomTitleis the workspace title, not the workspace-tab title, so renaming the only workspace tab still resets on restore.💡 One way to preserve the legacy shape unless per-tab-only state exists
- let tabSnapshots: [SessionWorkspaceTabSnapshot]? = workspaceTabs.count > 1 + let shouldPersistWorkspaceTabs = + workspaceTabs.count > 1 || + workspaceTabs.contains { + ($0.customTitle?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "").isEmpty == false + } + let tabSnapshots: [SessionWorkspaceTabSnapshot]? = shouldPersistWorkspaceTabs ? workspaceTabs.map { sessionWorkspaceTabSnapshot(tab: $0, includeScrollback: includeScrollback) } : nil ... - selectedWorkspaceTabIndex: workspaceTabs.count > 1 ? selectedWorkspaceTabIndex : nil + selectedWorkspaceTabIndex: shouldPersistWorkspaceTabs ? selectedWorkspaceTabIndex : nilAlso applies to: 258-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 240 - 243, Currently tab snapshots are only built when workspaceTabs.count > 1, which drops per-tab-only state (like a lone tab's customTitle) for single-tab workspaces; update the logic so that you call sessionWorkspaceTabSnapshot(tab:includeScrollback:) for the lone tab as well whenever any tab contains per-tab-only state (e.g. customTitle) — i.e. compute a boolean like hasPerTabState = workspaceTabs.contains { $0.customTitle != nil /* or other per-tab-only props */ } and then set tabSnapshots = workspaceTabs.map { sessionWorkspaceTabSnapshot(tab: $0, includeScrollback: includeScrollback) } if workspaceTabs.count > 1 || hasPerTabState, otherwise nil.
🧹 Nitpick comments (5)
Sources/cmuxApp.swift (2)
168-173: Factor the new shortcut plumbing through one helper.These new storage/accessor pairs all repeat the same decode/fallback shape. A tiny wrapper around
decodeShortcut(from:fallback:)would reduce the copy/paste surface and make the next shortcut addition less error-prone.♻️ Example simplification
+ private func menuShortcut(from data: Data, action: KeyboardShortcutSettings.Action) -> StoredShortcut { + decodeShortcut(from: data, fallback: action.defaultShortcut) + } + private var nextWorkspaceTabMenuShortcut: StoredShortcut { - decodeShortcut( - from: nextWorkspaceTabShortcutData, - fallback: KeyboardShortcutSettings.Action.nextWorkspaceTab.defaultShortcut - ) + menuShortcut(from: nextWorkspaceTabShortcutData, action: .nextWorkspaceTab) }Also applies to: 994-1034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 168 - 173, Multiple `@AppStorage` Data properties repeat the same decode/fallback pattern; introduce a small helper that wraps decodeShortcut(from:fallback:) and use it when initializing/reading these shortcuts. Create a function (e.g. loadShortcut(fromData:fallback:) or a computed property initializer) that takes the Data AppStorage value (nextWorkspaceTabShortcutData, prevWorkspaceTabShortcutData, newWorkspaceTabShortcutData, renameWorkspaceTabShortcutData, closeWorkspaceTabShortcutData, closeOtherTabsInPaneShortcutData) and returns the decoded KeyboardShortcut with the provided fallback, then replace the repeated decode/fallback calls with calls to that helper (referencing decodeShortcut(from:fallback:) inside it) to remove duplication.
790-808: Disable the workspace-tab navigation/close commands when only one tab exists.
Next,Previous, andClose Workspace Tabstay enabled here even though the rest of the feature suppresses those actions for single-tab workspaces. That leaves dead menu items at best, and it can expose a last-tab close path that the tab bar intentionally hides. Please gate these on the selected workspace’s tab count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 790 - 808, The Next/Previous/Close workspace tab commands should be disabled when the selected workspace has only one tab; update the splitCommandButton invocations that call activeTabManager.selectNextWorkspaceTab, selectPreviousWorkspaceTab, and closeSelectedWorkspaceTab to compute a boolean like hasMultipleTabs = (activeTabManager.selectedWorkspace?.tabs.count ?? 0) > 1 (or use an existing helper on activeTabManager) and apply that to the button’s disabled state (e.g., .disabled(!hasMultipleTabs) or the equivalent API) so the commands are not enabled for single-tab workspaces.Sources/AppDelegate.swift (2)
4402-4412: Make the helper’s defaultsourceAPI-neutral.The sibling
requestCommandPalette*helpers default toapi.*, but this one defaults to a shortcut-specific source. If another caller reuses it later, the provenance is wrong by default. I’d make the helper defaultapi.commandPaletteRenameWorkspaceTaband pass"shortcut.renameWorkspaceTab"explicitly from the shortcut branch.Suggested tweak
func requestCommandPaletteRenameWorkspaceTab( preferredWindow: NSWindow? = nil, - source: String = "shortcut.renameWorkspaceTab" + source: String = "api.commandPaletteRenameWorkspaceTab" ) { postCommandPaletteRequest( name: .commandPaletteRenameWorkspaceTabRequested, preferredWindow: preferredWindow, source: source, markPending: true ) } if matchShortcut(event: event, shortcut: KeyboardShortcutSettings.shortcut(for: .renameWorkspaceTab)) { requestCommandPaletteRenameWorkspaceTab( - preferredWindow: commandPaletteTargetWindow ?? event.window ?? NSApp.keyWindow ?? NSApp.mainWindow + preferredWindow: commandPaletteTargetWindow ?? event.window ?? NSApp.keyWindow ?? NSApp.mainWindow, + source: "shortcut.renameWorkspaceTab" ) return true }Also applies to: 9420-9423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4402 - 4412, The helper requestCommandPaletteRenameWorkspaceTab has a shortcut-specific default for its source parameter ("shortcut.renameWorkspaceTab") which makes provenance incorrect for non-shortcut callers; change the default source to an API-neutral value (e.g., "api.commandPaletteRenameWorkspaceTab") and update the shortcut-specific caller to pass "shortcut.renameWorkspaceTab" explicitly when invoking postCommandPaletteRequest; ensure the same change is applied to the other duplicate helper occurrence (the other requestCommandPaletteRenameWorkspaceTab definition) so both use the neutral default and the shortcut path supplies the explicit shortcut source.
9404-9404: Fix the shortcut comment before it drifts further.Line 9404 says
Cmd+Shift+T, but.newWorkspaceTabdefaults to⌥⌘TinKeyboardShortcutSettings. I’d either correct the binding in the comment or make the comment generic.Suggested tweak
- // Workspace tab navigation: Cmd+Opt+] / Cmd+Opt+[ / Cmd+Shift+T + // Workspace tab navigation/management shortcuts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` at line 9404, The comment "Workspace tab navigation: Cmd+Opt+] / Cmd+Opt+[ / Cmd+Shift+T" is inconsistent with the actual default binding for .newWorkspaceTab in KeyboardShortcutSettings (which is ⌥⌘T); update the comment to match the real default or make it generic. Locate the comment near the workspace tab navigation section in AppDelegate.swift and either change "Cmd+Shift+T" to "Cmd+Opt+T" (or to "⌥⌘T") or replace the specific binding with a generic phrase like "use your configured shortcut (see KeyboardShortcutSettings.newWorkspaceTab)"; ensure you reference .newWorkspaceTab/KeyboardShortcutSettings in the updated comment so it stays accurate.Sources/WorkspaceTab.swift (1)
82-153: This bookkeeping wants a couple of smaller substate types.From Line 82 onward,
WorkspaceTabis already carrying several distinct state machines: close confirmation, focus reconciliation, detach/attach transfer, and layout follow-up. Pulling those into dedicated nested structs/helpers would make the invariants easier to follow and reduce the risk of this extraction turning into another catch-all model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceTab.swift` around lines 82 - 153, WorkspaceTab currently mixes multiple independent substates in-line (close confirmation, focus reconciliation, detach/attach, layout follow-up) making invariants hard to follow; extract these into small nested types (e.g., WorkspaceTab.CloseState, WorkspaceTab.FocusReconcileState, WorkspaceTab.DetachState, WorkspaceTab.LayoutFollowUpState) and move the related stored properties into each respective struct (for example move pendingCloseConfirmTabIds, forceCloseTabIds, explicitUserCloseTabIds, postCloseSelectTabId into CloseState; isReconcilingFocusState, focusReconcileScheduled and related debug counters into FocusReconcileState; detachingTabIds, pendingDetachedSurfaces, activeDetachCloseTransactions into DetachState; layoutFollowUpObservers, layoutFollowUpPanelsCancellable, layoutFollowUpTimeoutWorkItem, layoutFollowUpReason, layoutFollowUpTerminalFocusPanelId, layoutFollowUpBrowserPanelId, layoutFollowUpBrowserExitFocusPanelId, layoutFollowUpNeedsGeometryPass, layoutFollowUpAttemptScheduled, layoutFollowUpStalledAttemptCount, isAttemptingLayoutFollowUp, layoutFollowUpStalledAttemptCount, layoutFollowUpObservers into LayoutFollowUpState). Keep external behaviour by replacing the moved properties on WorkspaceTab with computed property accessors that forward to the nested structs and update initializers/usages (e.g., references to pendingDetachedSurfaces, pendingCloseConfirmTabIds, isReconcilingFocusState, layoutFollowUpReason) to the new nested types; ensure Codable/Published/AnyCancellable semantics are preserved (keep `@Published` where required) and run tests to fix any remaining call-sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 5885-5899: The new tab code is instantiating TerminalPanel
directly which creates local terminals for remote (SSH) workspaces; replace this
direct creation in createWorkspaceTab() with the existing remote-aware terminal
creation path by invoking the same helper used elsewhere (e.g.
newTerminalSurface(...) or the code that applies remoteTerminalStartupCommand()
and trackRemoteTerminalSurface()), so that remote startup commands and
remote-session tracking are applied; ensure you stop directly calling
TerminalPanel(...) and instead use the helper that returns/configures the panel,
then call configureTerminalPanel(...), set tab.panels[...] and
tab.panelTitles[...] with the returned panel id/displayTitle and still call
seedTerminalInheritanceFontPoints(...) as before.
In `@Sources/WorkspaceContentView.swift`:
- Around line 273-335: Hidden/ background WorkspaceTab instances are not having
their bonsplit metadata refreshed because syncBonsplitNotificationBadges() only
inspects workspace.bonsplitController and workspace.manualUnreadPanelIds; update
the sync to iterate per-tab controllers. Change syncBonsplitNotificationBadges()
(or add a new helper) to walk workspace.workspaceTabs and for each wsTab use
wsTab.bonsplitController and wsTab.manualUnreadPanelIds to update
badges/pins/kind, or trigger a per-tab sync when selectedWorkspaceTabIndex
changes so background tabs' metadata is refreshed when they become visible.
- Line 330: The remount key for each BonsplitView is currently built with
splitZoomRenderIdentity derived from workspace, causing zoom changes to remount
every stacked view; instead compute the zoom identity scoped to the wsTab and
use that in the .id. Replace
.id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)") with a per-tab value
(e.g., read wsTab.bonsplitController.zoomedPaneId or the equivalent zoom ID on
wsTab) and use .id("\(wsTab.id.uuidString)-\(wsTabZoomId)") so only the affected
tab remounts, leaving hidden tabs alive.
In `@Sources/WorkspaceTab.swift`:
- Around line 25-26: Add a read-only computed property displayTitle on the
WorkspaceTab model that returns a non-empty, trimmed title: if customTitle
(trimmed) is non-empty return it, otherwise return the localized default e.g.
String(localized: "workspace.new-tab", defaultValue: "New Tab"); update any call
sites that currently do customTitle ?? derivedTitle to use displayTitle (or
derivedTitle where appropriate) so UI never receives an empty string; ensure
localization API and trimming are used inside the displayTitle implementation
and keep customTitle as-is for editing purposes.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 5241-5244: The palette context currently only records tab count
via snapshot.setBool(CommandPaletteContextKeys.workspaceHasMultipleTabs,
workspace.workspaceTabs.count > 1) so commands still display the parent
workspaceName instead of the selected tab; update the context to also capture
the active workspace-tab name (e.g., set a new or existing key like
CommandPaletteContextKeys.workspaceTabName or similar) with the selected tab's
name (derived from workspace.workspaceTabs[workspace.activeTabIndex] or the
workspace.activeTab property) and ensure the same change is applied where
workspaceName is read (around the code that handles the
Close/Next/Previous/Rename Workspace Tab entries at the 5307-5310 area) so those
commands show the selected tab name instead of the parent workspace.
In `@Sources/Workspace.swift`:
- Around line 6087-6102: syncActivePanels() replaces the activePanels publisher
but doesn't refresh focus-derived mirrors (currentDirectory, gitBranch,
pullRequest); update it to re-use the existing tab-selection path by calling the
same focus-refresh routine used on tab selection (e.g. invoke
reassertWorkspaceTabFocus() or the shared tab-selection handler) immediately
after swapping tab and setting activePanels so responder-derived mirrors are
recomputed; ensure the same change is applied in the sibling block noted (the
duplicate at 6104-6121) so both paths keep mirror state in sync when tabs are
switched/closed/restored.
- Around line 240-243: Currently tab snapshots are only built when
workspaceTabs.count > 1, which drops per-tab-only state (like a lone tab's
customTitle) for single-tab workspaces; update the logic so that you call
sessionWorkspaceTabSnapshot(tab:includeScrollback:) for the lone tab as well
whenever any tab contains per-tab-only state (e.g. customTitle) — i.e. compute a
boolean like hasPerTabState = workspaceTabs.contains { $0.customTitle != nil /*
or other per-tab-only props */ } and then set tabSnapshots = workspaceTabs.map {
sessionWorkspaceTabSnapshot(tab: $0, includeScrollback: includeScrollback) } if
workspaceTabs.count > 1 || hasPerTabState, otherwise nil.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 4402-4412: The helper requestCommandPaletteRenameWorkspaceTab has
a shortcut-specific default for its source parameter
("shortcut.renameWorkspaceTab") which makes provenance incorrect for
non-shortcut callers; change the default source to an API-neutral value (e.g.,
"api.commandPaletteRenameWorkspaceTab") and update the shortcut-specific caller
to pass "shortcut.renameWorkspaceTab" explicitly when invoking
postCommandPaletteRequest; ensure the same change is applied to the other
duplicate helper occurrence (the other requestCommandPaletteRenameWorkspaceTab
definition) so both use the neutral default and the shortcut path supplies the
explicit shortcut source.
- Line 9404: The comment "Workspace tab navigation: Cmd+Opt+] / Cmd+Opt+[ /
Cmd+Shift+T" is inconsistent with the actual default binding for
.newWorkspaceTab in KeyboardShortcutSettings (which is ⌥⌘T); update the comment
to match the real default or make it generic. Locate the comment near the
workspace tab navigation section in AppDelegate.swift and either change
"Cmd+Shift+T" to "Cmd+Opt+T" (or to "⌥⌘T") or replace the specific binding with
a generic phrase like "use your configured shortcut (see
KeyboardShortcutSettings.newWorkspaceTab)"; ensure you reference
.newWorkspaceTab/KeyboardShortcutSettings in the updated comment so it stays
accurate.
In `@Sources/cmuxApp.swift`:
- Around line 168-173: Multiple `@AppStorage` Data properties repeat the same
decode/fallback pattern; introduce a small helper that wraps
decodeShortcut(from:fallback:) and use it when initializing/reading these
shortcuts. Create a function (e.g. loadShortcut(fromData:fallback:) or a
computed property initializer) that takes the Data AppStorage value
(nextWorkspaceTabShortcutData, prevWorkspaceTabShortcutData,
newWorkspaceTabShortcutData, renameWorkspaceTabShortcutData,
closeWorkspaceTabShortcutData, closeOtherTabsInPaneShortcutData) and returns the
decoded KeyboardShortcut with the provided fallback, then replace the repeated
decode/fallback calls with calls to that helper (referencing
decodeShortcut(from:fallback:) inside it) to remove duplication.
- Around line 790-808: The Next/Previous/Close workspace tab commands should be
disabled when the selected workspace has only one tab; update the
splitCommandButton invocations that call
activeTabManager.selectNextWorkspaceTab, selectPreviousWorkspaceTab, and
closeSelectedWorkspaceTab to compute a boolean like hasMultipleTabs =
(activeTabManager.selectedWorkspace?.tabs.count ?? 0) > 1 (or use an existing
helper on activeTabManager) and apply that to the button’s disabled state (e.g.,
.disabled(!hasMultipleTabs) or the equivalent API) so the commands are not
enabled for single-tab workspaces.
In `@Sources/WorkspaceTab.swift`:
- Around line 82-153: WorkspaceTab currently mixes multiple independent
substates in-line (close confirmation, focus reconciliation, detach/attach,
layout follow-up) making invariants hard to follow; extract these into small
nested types (e.g., WorkspaceTab.CloseState, WorkspaceTab.FocusReconcileState,
WorkspaceTab.DetachState, WorkspaceTab.LayoutFollowUpState) and move the related
stored properties into each respective struct (for example move
pendingCloseConfirmTabIds, forceCloseTabIds, explicitUserCloseTabIds,
postCloseSelectTabId into CloseState; isReconcilingFocusState,
focusReconcileScheduled and related debug counters into FocusReconcileState;
detachingTabIds, pendingDetachedSurfaces, activeDetachCloseTransactions into
DetachState; layoutFollowUpObservers, layoutFollowUpPanelsCancellable,
layoutFollowUpTimeoutWorkItem, layoutFollowUpReason,
layoutFollowUpTerminalFocusPanelId, layoutFollowUpBrowserPanelId,
layoutFollowUpBrowserExitFocusPanelId, layoutFollowUpNeedsGeometryPass,
layoutFollowUpAttemptScheduled, layoutFollowUpStalledAttemptCount,
isAttemptingLayoutFollowUp, layoutFollowUpStalledAttemptCount,
layoutFollowUpObservers into LayoutFollowUpState). Keep external behaviour by
replacing the moved properties on WorkspaceTab with computed property accessors
that forward to the nested structs and update initializers/usages (e.g.,
references to pendingDetachedSurfaces, pendingCloseConfirmTabIds,
isReconcilingFocusState, layoutFollowUpReason) to the new nested types; ensure
Codable/Published/AnyCancellable semantics are preserved (keep `@Published` where
required) and run tests to fix any remaining call-sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64d11e70-7ba8-477d-ae27-390fc0e7c4ec
📒 Files selected for processing (11)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftSources/WorkspaceTab.swiftSources/WorkspaceTabBarView.swiftSources/cmuxApp.swiftcmuxTests/SessionPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/SessionPersistence.swift
- Sources/KeyboardShortcutSettings.swift
- Sources/TabManager.swift
Comment on lines
+5885
to
+5899
| // Create a default terminal panel in the new tab using the workspace's | ||
| // current directory. | ||
| let terminalPanel = TerminalPanel( | ||
| workspaceId: id, | ||
| context: GHOSTTY_SURFACE_CONTEXT_TAB, | ||
| configTemplate: nil, | ||
| workingDirectory: currentDirectory, | ||
| portOrdinal: portOrdinal | ||
| ) | ||
| configureTerminalPanel(terminalPanel) | ||
| // Write directly to the tab (not through pass-throughs, which point to | ||
| // the selected tab -- which is now this new tab, but let's be explicit). | ||
| tab.panels[terminalPanel.id] = terminalPanel | ||
| tab.panelTitles[terminalPanel.id] = terminalPanel.displayTitle | ||
| seedTerminalInheritanceFontPoints(panelId: terminalPanel.id, configTemplate: nil) |
There was a problem hiding this comment.
createWorkspaceTab() is creating local terminals inside remote workspaces.
This path seeds the first TerminalPanel directly instead of reusing the remote-aware terminal creation logic. Unlike newTerminalSurface(...), it never applies remoteTerminalStartupCommand() or trackRemoteTerminalSurface(...), so a new workspace tab under an SSH workspace opens a local shell, can inherit a remote-only currentDirectory, and falls out of remote-session accounting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 5885 - 5899, The new tab code is
instantiating TerminalPanel directly which creates local terminals for remote
(SSH) workspaces; replace this direct creation in createWorkspaceTab() with the
existing remote-aware terminal creation path by invoking the same helper used
elsewhere (e.g. newTerminalSurface(...) or the code that applies
remoteTerminalStartupCommand() and trackRemoteTerminalSurface()), so that remote
startup commands and remote-session tracking are applied; ensure you stop
directly calling TerminalPanel(...) and instead use the helper that
returns/configures the panel, then call configureTerminalPanel(...), set
tab.panels[...] and tab.panelTitles[...] with the returned panel id/displayTitle
and still call seedTerminalInheritanceFontPoints(...) as before.
Comment on lines
+273
to
335
| ForEach(Array(workspace.workspaceTabs.enumerated()), id: \.element.id) { tabIndex, wsTab in | ||
| let isSelectedTab = tabIndex == workspace.selectedWorkspaceTabIndex | ||
| let isSplit = wsTab.bonsplitController.allPaneIds.count > 1 || | ||
| wsTab.panels.count > 1 | ||
|
|
||
| BonsplitView(controller: wsTab.bonsplitController) { tab, paneId in | ||
| let _ = Self.debugPanelLookup(tab: tab, workspace: workspace) | ||
| if let panel = wsTab.panels[wsTab.surfaceIdToPanelId[tab.id] ?? UUID()] { | ||
| let isFocused = isWorkspaceInputActive && isSelectedTab && workspace.focusedPanelId == panel.id | ||
| let isSelectedInPane = wsTab.bonsplitController.selectedTab(inPane: paneId)?.id == tab.id | ||
| let isVisibleInUI = Self.panelVisibleInUI( | ||
| isWorkspaceVisible: isWorkspaceVisible && isSelectedTab, | ||
| isSelectedInPane: isSelectedInPane, | ||
| isFocused: isFocused | ||
| ) | ||
| let showsNotificationRing = Workspace.shouldShowUnreadIndicator( | ||
| hasUnreadNotification: notificationStore.hasVisibleNotificationIndicator( | ||
| forTabId: workspace.id, | ||
| surfaceId: panel.id | ||
| ), | ||
| isManuallyUnread: wsTab.manualUnreadPanelIds.contains(panel.id) | ||
| ) | ||
| PanelContentView( | ||
| panel: panel, | ||
| paneId: paneId, | ||
| isFocused: isFocused, | ||
| isSelectedInPane: isSelectedInPane, | ||
| isVisibleInUI: isVisibleInUI, | ||
| portalPriority: isSelectedTab ? workspacePortalPriority : 0, | ||
| isSplit: isSplit, | ||
| appearance: appearance, | ||
| hasUnreadNotification: showsNotificationRing && !usesWorkspacePaneOverlay, | ||
| onFocus: { | ||
| guard isWorkspaceInputActive, isSelectedTab else { return } | ||
| guard wsTab.panels[panel.id] != nil else { return } | ||
| workspace.focusPanel(panel.id, trigger: .terminalFirstResponder) | ||
| }, | ||
| onRequestPanelFocus: { | ||
| guard isWorkspaceInputActive, isSelectedTab else { return } | ||
| guard wsTab.panels[panel.id] != nil else { return } | ||
| workspace.focusPanel(panel.id) | ||
| }, | ||
| onTriggerFlash: { workspace.triggerDebugFlash(panelId: panel.id) } | ||
| ) | ||
| .onTapGesture { | ||
| wsTab.bonsplitController.focusPane(paneId) | ||
| } | ||
| } else { | ||
| EmptyPanelView(workspace: workspace, paneId: paneId) | ||
| } | ||
| } emptyPane: { paneId in | ||
| EmptyPanelView(workspace: workspace, paneId: paneId) | ||
| .onTapGesture { | ||
| wsTab.bonsplitController.focusPane(paneId) | ||
| } | ||
| } | ||
| } else { | ||
| // Fallback for tabs without panels (shouldn't happen normally) | ||
| EmptyPanelView(workspace: workspace, paneId: paneId) | ||
| .internalOnlyTabDrag() | ||
| .id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)") | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| .opacity(isSelectedTab ? 1 : 0) | ||
| .allowsHitTesting(isSelectedTab) | ||
| .zIndex(isSelectedTab ? 1 : 0) | ||
| } |
There was a problem hiding this comment.
Hidden workspace tabs won't get their Bonsplit metadata refreshed.
This render path is tab-scoped now, but syncBonsplitNotificationBadges() below still only walks workspace.bonsplitController / workspace.manualUnreadPanelIds. Background WorkspaceTabs can keep stale badge, pin, and kind state until another sync happens after they become active. Please sync each wsTab.bonsplitController with the tab-local maps/sets, or at least resync on selectedWorkspaceTabIndex changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WorkspaceContentView.swift` around lines 273 - 335, Hidden/
background WorkspaceTab instances are not having their bonsplit metadata
refreshed because syncBonsplitNotificationBadges() only inspects
workspace.bonsplitController and workspace.manualUnreadPanelIds; update the sync
to iterate per-tab controllers. Change syncBonsplitNotificationBadges() (or add
a new helper) to walk workspace.workspaceTabs and for each wsTab use
wsTab.bonsplitController and wsTab.manualUnreadPanelIds to update
badges/pins/kind, or trigger a per-tab sync when selectedWorkspaceTabIndex
changes so background tabs' metadata is refreshed when they become visible.
| // Fallback for tabs without panels (shouldn't happen normally) | ||
| EmptyPanelView(workspace: workspace, paneId: paneId) | ||
| .internalOnlyTabDrag() | ||
| .id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "WorkspaceContentView id wiring:"
rg -n -C2 'splitZoomRenderIdentity|\.id\(' Sources/WorkspaceContentView.swift
echo
echo "Workspace.bonsplitController / activeWorkspaceTab wiring:"
rg -n -C3 'var bonsplitController|activeWorkspaceTab' Sources --type=swiftRepository: manaflow-ai/cmux
Length of output: 23709
🏁 Script executed:
rg -n 'class WorkspaceTab|struct WorkspaceTab' Sources --type=swift -A 20Repository: manaflow-ai/cmux
Length of output: 5830
🏁 Script executed:
rg -n 'var bonsplitController' Sources/WorkspaceTab.swift --type=swift -B 2 -A 3Repository: manaflow-ai/cmux
Length of output: 42
Scope the remount key to wsTab, not workspace.
splitZoomRenderIdentity still comes from workspace.bonsplitController.zoomedPaneId, so zooming the selected workspace tab—or switching to one with a different zoom state—changes the .id for every stacked BonsplitView. That tears down hidden tabs and defeats the keep-alive/anti-ghosting strategy this ZStack is trying to preserve.
🔧 Proposed fix
- .id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)")
+ .id("\(wsTab.id.uuidString)-\(wsTab.bonsplitController.zoomedPaneId.map { "zoom:\($0.id.uuidString)" } ?? "unzoomed")")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)") | |
| .id("\(wsTab.id.uuidString)-\(wsTab.bonsplitController.zoomedPaneId.map { "zoom:\($0.id.uuidString)" } ?? "unzoomed")") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WorkspaceContentView.swift` at line 330, The remount key for each
BonsplitView is currently built with splitZoomRenderIdentity derived from
workspace, causing zoom changes to remount every stacked view; instead compute
the zoom identity scoped to the wsTab and use that in the .id. Replace
.id("\(wsTab.id.uuidString)-\(splitZoomRenderIdentity)") with a per-tab value
(e.g., read wsTab.bonsplitController.zoomedPaneId or the equivalent zoom ID on
wsTab) and use .id("\(wsTab.id.uuidString)-\(wsTabZoomId)") so only the affected
tab remounts, leaving hidden tabs alive.
Comment on lines
+25
to
+26
| /// User-set custom title for this tab. When nil, the tab bar derives a title from panel state. | ||
| @Published var customTitle: String? |
There was a problem hiding this comment.
Expose one non-empty display title from the model.
Line 213 falls back to "", and customTitle can also be blank. A caller using customTitle ?? derivedTitle can therefore render an invisible tab label for a fresh tab or cleared rename input. I'd expose a read-side displayTitle that trims customTitle and applies the localized "New Tab" fallback once.
💡 Possible fix
+ var displayTitle: String {
+ if let customTitle = customTitle?.trimmingCharacters(in: .whitespacesAndNewlines),
+ !customTitle.isEmpty {
+ return customTitle
+ }
+ return derivedTitle.isEmpty
+ ? String(localized: "workspaceTab.new", defaultValue: "New Tab")
+ : derivedTitle
+ }
+
/// The derived display title for this tab, preferring the focused panel's directory.
var derivedTitle: String {Also applies to: 191-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WorkspaceTab.swift` around lines 25 - 26, Add a read-only computed
property displayTitle on the WorkspaceTab model that returns a non-empty,
trimmed title: if customTitle (trimmed) is non-empty return it, otherwise return
the localized default e.g. String(localized: "workspace.new-tab", defaultValue:
"New Tab"); update any call sites that currently do customTitle ?? derivedTitle
to use displayTitle (or derivedTitle where appropriate) so UI never receives an
empty string; ensure localization API and trimming are used inside the
displayTitle implementation and keep customTitle as-is for editing purposes.
Author
|
This is my first PR, please give feedback or suggestions if anything needs changing! |
Merged upstream changes including: - Niri canvas experimental feature (NiriCanvasView) - Ghostty submodule sync - Remote terminal child exit tracking - cmux config custom commands in command palette - Titlebar double-click monitor All conflicts resolved preserving both workspace tabs code and upstream additions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TabManager.swift (1)
1327-1358:⚠️ Potential issue | 🟡 MinorPin the welcome command to the original terminal panel.
finishIfReady()and the readiness observer both resolve throughworkspace.focusedTerminalPanel. With workspace tabs, the active tab can change before the initial terminal becomes ready, so the delayedcmux welcomecan be sent to a different tab and still mark onboarding as shown. Capture the original workspace-tab/panel target before subscribing and gate readiness against that exact target instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1327 - 1358, finishIfReady() currently uses workspace.focusedTerminalPanel which can change; capture the initial target terminal panel (e.g., let initialPanel = workspace.focusedTerminalPanel) before creating panelsCancellable/readyObserver, then change finishIfReady() and the notification closure to check and act only if the panel equals that captured initialPanel (and its surface is non-nil). Also use the captured initialPanel when sending "cmux welcome\n" and when removing observers/cancelling so the onboarding is pinned to the original terminal panel.Sources/AppDelegate.swift (1)
6356-6367:⚠️ Potential issue | 🟡 MinorThis can send the queued text to the wrong workspace tab.
Workspace.panelsPublishermirrors the selected workspace tab’s panels, andfinishIfReady()resolvestab.focusedTerminalPanelat send time. If the user switches workspace tabs before the original terminal becomes ready, the queued input follows the new selection and lands there instead of in the tab that spawned the request. Capture the target workspace-tab/panel up front and wait on that fixed target instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6356 - 6367, The current subscription uses tab.panelsPublisher and the terminalSurfaceDidBecomeReady notification which resolve tab.focusedTerminalPanel at send time, so if the user switches tabs the queued input may go to the wrong workspace; change the logic to capture the intended workspace tab and target panel up front (e.g. store a reference or unique identifier from tab and the specific Panel/TerminalPanel instance you intend to target) and then wait for readiness for that fixed target instead of responding to the selected tab’s panelsPublisher—replace the panelsPublisher sink and the generic notification handler in the panelsCancellable/readyObserver setup with observers that filter by the captured panel identifier or directly observe that panel’s readiness, and have finishIfReady() operate on the captured panel reference rather than tab.focusedTerminalPanel.
♻️ Duplicate comments (1)
Sources/TabManager.swift (1)
5484-5488:⚠️ Potential issue | 🟠 MajorHash the persisted workspace-tab snapshot, not just counts.
This closes the original create/close hole, but it still misses non-count mutations that the nested workspace-tab snapshot persists. A non-selected workspace tab can change its independent split/layout state while
workspaceTabs.count,selectedWorkspaceTabIndex, andwsTab.panels.countall stay the same, so autosave can still skip a dirty session. The safest fix is to derive this fingerprint from the same per-tab snapshot data thatsessionSnapshot(...)persists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5484 - 5488, The current hashing only uses counts (workspace.workspaceTabs.count, workspace.selectedWorkspaceTabIndex, wsTab.panels.count) which misses non-count mutations; instead, derive the workspace fingerprint from the same per-tab snapshot data used by sessionSnapshot(...). Replace the per-tab hasher.combine(wsTab.panels.count) (and the other count-only combines) with a loop that produces each tab's persisted snapshot (call the existing sessionSnapshot(...) or the helper that the saver uses for a workspace tab) and feed that snapshot (or its stable representation) into hasher.combine so any nested/layout changes will change the fingerprint; keep workspace.selectedWorkspaceTabIndex combined as well but stop relying on counts alone.
🧹 Nitpick comments (2)
Sources/cmuxApp.swift (1)
777-795: Consider matching the one-tab affordance in the View menu.
Next/Previous/Close Workspace Tabstay available even when the tab bar auto-hides and the command palette suppresses those actions for a single-tab workspace. Disabling or conditionally showing them here would avoid dead menu items and shortcuts in that state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 777 - 795, The Next/Previous/Close Workspace Tab menu entries (the splitCommandButton calls that call activeTabManager.selectNextWorkspaceTab(), selectPreviousWorkspaceTab(), and closeSelectedWorkspaceTab()) remain enabled for single-tab workspaces and should be disabled or hidden to match the command-palette behavior; update those splitCommandButton invocations to conditionally disable (or not show) the menu items and their shortcuts when the current workspace has only one tab — e.g., query the activeTabManager for the selected workspace tab count (selected workspace / selectedWorkspace?.tabs.count or an equivalent API on activeTabManager) and return disabled/hidden for count <= 1; apply the same conditional visibility/enablement logic for the Close Workspace Tab entry (and optionally for Rename if you choose) so shortcuts are not active in single-tab workspaces.Sources/AppDelegate.swift (1)
9587-9587: Avoid hard-coding concrete key combos in comments here.These handlers already read from
KeyboardShortcutSettings, so an inline shortcut note will drift as soon as defaults change or users remap the binding. An action-only comment would age better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` at line 9587, Replace the hard-coded key combo text in the comment with an action-only note that references the shortcut configuration (e.g., change "// Workspace tab navigation: Cmd+Opt+] / Cmd+Opt+[ / Cmd+Shift+T" to something like "// Workspace tab navigation (uses KeyboardShortcutSettings)"); update any nearby comments that mention concrete keys so they instead point to KeyboardShortcutSettings or the specific handlers that read those settings (the workspace tab navigation handlers) so the comment won't drift when defaults or user bindings change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 6356-6367: The current subscription uses tab.panelsPublisher and
the terminalSurfaceDidBecomeReady notification which resolve
tab.focusedTerminalPanel at send time, so if the user switches tabs the queued
input may go to the wrong workspace; change the logic to capture the intended
workspace tab and target panel up front (e.g. store a reference or unique
identifier from tab and the specific Panel/TerminalPanel instance you intend to
target) and then wait for readiness for that fixed target instead of responding
to the selected tab’s panelsPublisher—replace the panelsPublisher sink and the
generic notification handler in the panelsCancellable/readyObserver setup with
observers that filter by the captured panel identifier or directly observe that
panel’s readiness, and have finishIfReady() operate on the captured panel
reference rather than tab.focusedTerminalPanel.
In `@Sources/TabManager.swift`:
- Around line 1327-1358: finishIfReady() currently uses
workspace.focusedTerminalPanel which can change; capture the initial target
terminal panel (e.g., let initialPanel = workspace.focusedTerminalPanel) before
creating panelsCancellable/readyObserver, then change finishIfReady() and the
notification closure to check and act only if the panel equals that captured
initialPanel (and its surface is non-nil). Also use the captured initialPanel
when sending "cmux welcome\n" and when removing observers/cancelling so the
onboarding is pinned to the original terminal panel.
---
Duplicate comments:
In `@Sources/TabManager.swift`:
- Around line 5484-5488: The current hashing only uses counts
(workspace.workspaceTabs.count, workspace.selectedWorkspaceTabIndex,
wsTab.panels.count) which misses non-count mutations; instead, derive the
workspace fingerprint from the same per-tab snapshot data used by
sessionSnapshot(...). Replace the per-tab hasher.combine(wsTab.panels.count)
(and the other count-only combines) with a loop that produces each tab's
persisted snapshot (call the existing sessionSnapshot(...) or the helper that
the saver uses for a workspace tab) and feed that snapshot (or its stable
representation) into hasher.combine so any nested/layout changes will change the
fingerprint; keep workspace.selectedWorkspaceTabIndex combined as well but stop
relying on counts alone.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Line 9587: Replace the hard-coded key combo text in the comment with an
action-only note that references the shortcut configuration (e.g., change "//
Workspace tab navigation: Cmd+Opt+] / Cmd+Opt+[ / Cmd+Shift+T" to something like
"// Workspace tab navigation (uses KeyboardShortcutSettings)"); update any
nearby comments that mention concrete keys so they instead point to
KeyboardShortcutSettings or the specific handlers that read those settings (the
workspace tab navigation handlers) so the comment won't drift when defaults or
user bindings change.
In `@Sources/cmuxApp.swift`:
- Around line 777-795: The Next/Previous/Close Workspace Tab menu entries (the
splitCommandButton calls that call activeTabManager.selectNextWorkspaceTab(),
selectPreviousWorkspaceTab(), and closeSelectedWorkspaceTab()) remain enabled
for single-tab workspaces and should be disabled or hidden to match the
command-palette behavior; update those splitCommandButton invocations to
conditionally disable (or not show) the menu items and their shortcuts when the
current workspace has only one tab — e.g., query the activeTabManager for the
selected workspace tab count (selected workspace / selectedWorkspace?.tabs.count
or an equivalent API on activeTabManager) and return disabled/hidden for count
<= 1; apply the same conditional visibility/enablement logic for the Close
Workspace Tab entry (and optionally for Rename if you choose) so shortcuts are
not active in single-tab workspaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dc319b0-256d-42c6-945f-6ca76cdbd8fd
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/ContentView.swiftSources/TabManager.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftSources/cmuxApp.swiftcmuxTests/SessionPersistenceTests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/WorkspaceContentView.swift
- Sources/ContentView.swift
- Sources/Workspace.swift
Author
|
All code review bot feedback has been addressed in the second commit: Critical fixes:
Major fixes:
Minor fixes:
Merge conflicts with upstream main have also been resolved. Build compiles clean and all existing unit tests pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a tab layer between workspaces (sidebar) and split panes, so each workspace can have multiple independent layouts — e.g., one tab with a fullscreen browser, another with a 6-pane terminal grid.
WorkspaceTabmodel — extracts BonsplitController + panel state from Workspace with computed pass-throughs for backward compatibility⌥⌘](next),⌥⌘[(prev),⌥⌘T(new),⌥⌘R(rename),⌥⌘W(close)===), preventing cross-tab state corruptionactivePanelsmirror stays connected across tab switchesselectedWorkspaceTab!replaced withactiveWorkspaceTab(descriptive fatalError)workspaceTabsarray onSessionWorkspaceSnapshotTest plan
xcodebuild, 0 errors)selectedWorkspaceTab!force-unwrapsworkspaceTab(for:)🤖 Generated with Claude Code
Summary by cubic
Adds workspace tabs: a nested tab layer inside each workspace so users can keep multiple, independent split layouts per workspace. Improves tab switching stability, persistence, and shortcuts, and integrates cleanly with the latest upstream changes.
New Features
Bug Fixes
KeyboardShortcutSettings.Written for commit 02aa81d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests